Add telemetry config management with related UI dialog and settings#6028
Add telemetry config management with related UI dialog and settings#6028zanesq merged 14 commits intomicn/posthogfrom
Conversation
|
|
||
| useEffect(() => { | ||
| if (!provider || loadingModels || model || isCustomModel) return; | ||
| // Don't auto-select if user explicitly cleared the model |
There was a problem hiding this comment.
noticed a few usability edge cases with the model selector preselection - couldn't clear it easily and needed a better loading state
There was a problem hiding this comment.
roger that. still though, maybe revisit. this is a complex if statement now
| import TelemetryOptOutModal from '../../TelemetryOptOutModal'; | ||
|
|
||
| interface TelemetrySettingsProps { | ||
| isWelcome?: boolean; |
There was a problem hiding this comment.
onboarding needs different styling and html structure to match the other panels
There was a problem hiding this comment.
still though, let's stay away from optional booleans - better to be explicit
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
left some commnets - I know we're in a rush so could address in a follow up. mostly we should just use the normal way of handling settings, no need for separate environment checking and a separate route etc
| /// - GOOSE_TELEMETRY_ENABLED config value is set to false | ||
| /// | ||
| /// Returns true otherwise (telemetry is opt-out, enabled by default) | ||
| pub fn is_telemetry_enabled() -> bool { |
There was a problem hiding this comment.
to be honest, I don't think you need any of this. our config already reads environment variables and you can already read the config from the client. so no need for a special handler here or routes I would think
There was a problem hiding this comment.
ah thanks I didn't realize it did all that, will look into refactoring quickly 👍
| import TelemetryOptOutModal from '../../TelemetryOptOutModal'; | ||
|
|
||
| interface TelemetrySettingsProps { | ||
| isWelcome?: boolean; |
There was a problem hiding this comment.
still though, let's stay away from optional booleans - better to be explicit
|
|
||
| useEffect(() => { | ||
| if (!provider || loadingModels || model || isCustomModel) return; | ||
| // Don't auto-select if user explicitly cleared the model |
There was a problem hiding this comment.
roger that. still though, maybe revisit. this is a complex if statement now
| export default function TelemetryOptOutModal({ | ||
| isOpen: controlledIsOpen, | ||
| onClose, | ||
| showOnFirstLaunch = true, |
There was a problem hiding this comment.
hmm, also here. optional boolean and then default to true, just make it explicit
| interface TelemetryOptOutModalProps { | ||
| isOpen?: boolean; | ||
| onClose?: () => void; | ||
| showOnFirstLaunch?: boolean; |
There was a problem hiding this comment.
do any of these need to be optional?
| const checkTelemetryChoice = async () => { | ||
| try { | ||
| // First check if user has a provider configured (existing user) | ||
| const providerResponse = await readConfig({ |
There was a problem hiding this comment.
we have a hook for this we should use
| }); | ||
|
|
||
| // If the config value is null/undefined, user hasn't made a choice yet | ||
| if (telemetryResponse.data === null || telemetryResponse.data === undefined) { |
| setShowModal(true); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to check telemetry config:', error); |
There was a problem hiding this comment.
eh let's not drop errors on the floor
| @@ -0,0 +1,134 @@ | |||
| import { useState, useEffect } from 'react'; | |||
There was a problem hiding this comment.
this file could do with a quick look over on comments
| const title = 'Privacy'; | ||
| const description = 'Control how your data is used'; | ||
| const toggleLabel = 'Anonymous usage data'; | ||
| const toggleDescription = 'Help improve goose by sharing anonymous usage statistics.'; |
There was a problem hiding this comment.
any reason not to inline these?
There was a problem hiding this comment.
because they are reused below, would have to repeat the text twice leading to potentially out of sync later
2 participants
Summary
Add telemetry config management with related UI dialog and settings saved to config.
Existing users see modal, onboarding and settings has inline panel with learn more to launch the modal.